Skip to content

[SPARK-15615][SQL][BUILD][FOLLOW-UP] Replace deprecated usage of json(RDD[String]) API#17071

Closed
HyukjinKwon wants to merge 1 commit intoapache:masterfrom
HyukjinKwon:SPARK-15615-followup
Closed

[SPARK-15615][SQL][BUILD][FOLLOW-UP] Replace deprecated usage of json(RDD[String]) API#17071
HyukjinKwon wants to merge 1 commit intoapache:masterfrom
HyukjinKwon:SPARK-15615-followup

Conversation

@HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Feb 26, 2017

What changes were proposed in this pull request?

This PR proposes to replace the deprecated json(RDD[String]) usage to json(Dataset[String]).

This currently produces so many warnings.

How was this patch tested?

Fixed tests.

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Feb 26, 2017

Let me please cc both @cloud-fan and @srowen

@HyukjinKwon HyukjinKwon changed the title [SPARK-15615][SQL][BUILD][FOLLOW-UP] Remove deprecated usage of json(RDD[String]) API [SPARK-15615][SQL][BUILD][FOLLOW-UP] Replace deprecated usage of json(RDD[String]) API Feb 26, 2017
cloud-fan
cloud-fan reviewed Feb 26, 2017
View reviewed changes
examples/src/main/scala/org/apache/spark/examples/sql/SQLDataSourceExample.scala Outdated
Copy link
Contributor

@cloud-fan cloud-fan Feb 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use spark.createDataset here?

cloud-fan
cloud-fan reviewed Feb 26, 2017
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonSuite.scala Outdated
Copy link
Contributor

@cloud-fan cloud-fan Feb 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we switch to DataFrameWriter with text format?

cloud-fan
cloud-fan reviewed Feb 26, 2017
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonSuite.scala Outdated
Copy link
Contributor

@cloud-fan cloud-fan Feb 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use Dataset.union?

HyukjinKwon
HyukjinKwon commented Feb 26, 2017
View reviewed changes
sql/hive/src/test/java/org/apache/spark/sql/hive/JavaMetastoreDataSourcesSuite.java Outdated
Copy link
Member Author

@HyukjinKwon HyukjinKwon Feb 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opps, let me fix this one too.

@SparkQA
Copy link

SparkQA commented Feb 26, 2017

Test build #73478 has finished for PR 17071 at commit 626ca25.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

srowen
srowen approved these changes Feb 26, 2017
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we shouldn't use the deprecated method in non-test code. Even a lot of the test occurrences can be updated. I suppose there should be at least one test that the deprecated method still works?

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Feb 26, 2017

Oh, no. I think I updated all instances. Let me maybe leave one case in JsonSuite.

@SparkQA
Copy link

SparkQA commented Feb 26, 2017

Test build #73483 has finished for PR 17071 at commit f25d1ab.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Feb 26, 2017

Test build #73484 has finished for PR 17071 at commit d746d78.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Feb 26, 2017

I left single usage in JavaSaveLoadSuite. I believe this should be fine for testing the deprecated ones.

@SparkQA
Copy link

SparkQA commented Feb 26, 2017

Test build #73490 has finished for PR 17071 at commit 4cef1c6.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Feb 26, 2017

Test build #73488 has finished for PR 17071 at commit 2e89259.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Feb 26, 2017

Test build #73489 has finished for PR 17071 at commit aa82df2.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Feb 27, 2017

I like it, though, regarding still testing the deprecated method -- maybe it's best to even have a test that is explicitly just for testing the old method? that may be clearer than just picking some test from among another batch to leave with the old behavior. It might mean actually adding one new small test case in the generic JSON test suite for this purpose. What do you think?

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Feb 27, 2017

Sure, sounds better and I can't find a reason to not follow. Let me maybe add single small Java one somewhere because the deprecated Java one calls the deprecated Scala one.

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Feb 27, 2017

(I put a test here - https://github.com/apache/spark/pull/17071/files#diff-7e47859dbd409cc39f2908615fbd07ffR419)

@SparkQA
Copy link

SparkQA commented Feb 27, 2017

Test build #73506 has finished for PR 17071 at commit 6f35ee3.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan
Copy link
Contributor

cloud-fan commented Feb 27, 2017

thanks, merging to master!

@asfgit asfgit closed this in 8a5a585 Feb 27, 2017
@HyukjinKwon HyukjinKwon deleted the SPARK-15615-followup branch January 2, 2018 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fan cloud-fan cloud-fan left review comments

@srowen srowen srowen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@HyukjinKwon @SparkQA @srowen @cloud-fan